-
-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sqs batch failure error stack #762
Sqs batch failure error stack #762
Conversation
What if instead of attaching the stack traces to the message we attach them to the error similar to how we do in const sqsPartialBatchFailureMiddlewareAfter = async (request) => {
...
const rejectedReasons = getRejectedReasons(response)
// If all messages were processed successfully, continue and let the messages be deleted by Lambda's native functionality
if (!rejectedReasons.length) return
...
const errorMessage = getErrorMessage(rejectedReasons)
const error = new Error(errorMessage)
error.originalErrors = rejectedReasons
throw error
}
...
}
const getRejectedReasons = (response) => {
return response
.filter((r) => r.status === 'rejected')
.map((r) => r.reason)
}
const getErrorMessage = (rejectedReasons) => {
return rejectedReasons.map(error => error.message).join('\n')
} |
@willfarrell I tried out your code changes. It does add the So instead of this, how about we modify the .stack property of the new Error like this: checkout the first part of accepted answer. |
Are you using |
Thanks for that! I have modified the PR according your suggestions and after testing it, I am able to receive following logs (while failing a batch):
|
@willfarrell, On a similar note, |
There is an error logging middleware that can allow you to transform and ship where you like. You have to write the code those. Open a new issue if you’d like to discuss further. |
@willfarrell |
I'm holding off on merging this just for a bit. It relates to another issue around handling errors from |
No problem! |
@shivang007 Please take a look at #770, new AWS feature removes the need for throwing an error, so will need a logger instead. Would like your feedback on how errors should be handled for reporting purposes. |
@willfarrell |
Thanks for sharing. I was thinking about that as well. I can see where both ways are valuable depending on the use case. a. An error happened while processing a message (push to DLQ) This v3 approach is much better for Looking at other use cases like API Gateway + Lambda it is expected the error is handled to return a "success" with a statusCode indicating an error. I imagine monitoring tools handle this use case and display properly. I would expect, give time and little nudging, monitoring tools will handle this use case as well. The v2 middlewares will work with v3 |
Ofcourse, eventually they will adapt! Also saw your V3 code at https://github.com/middyjs/middy/blob/release/3.x/packages/sqs-partial-batch-failure/index.js |
@willfarrell is this released on npm? |
I hope to have an alpha out for the end of the week. |
What does this implement/fix? Explain your changes.
It passes Error Stack along with message.
Does this close any currently open issues?
No
Any other comments?
Currently, if any worker gets shared by multiple methods, then it's very difficult (sometimes impossible without running locally) to pinpoint the exact location where the error occurs. Hence adding the original stack of the error.
Where has this been tested?
Node.js Versions: 14.17.6
Middy Versions: 2.5.3
AWS SDK Versions: ^2.1014.0